Skip to content

Stop requesting Observability API permissions for blueprint agents by default - #501

Open
Krishnadheeraj (DheerajPannala) wants to merge 6 commits into
mainfrom
users/DheerajPannala/skip-observability-permissions
Open

Krishnadheeraj (DheerajPannala) wants to merge 6 commits into
mainfrom
users/DheerajPannala/skip-observability-permissions

Conversation

@DheerajPannala

@DheerajPannala Krishnadheeraj (DheerajPannala) commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

a365 setup all no longer requests Observability API (Agent365.Observability.OtelWrite) permissions for blueprint agents in any auth mode, so those agents need no Observability admin consent. This also fixes setup all --agent-registration-only exiting 0 when registration fails.

This follows the 3P Dev Scale scrum decision to make the no-consent flow the main path rather than an opt-in flag. The PR originally added --skip-observability-permissions; that flag is gone.

Why

With microsoft/Agent365-nodejs#290 and microsoft/Agent365-Samples#339, agents export telemetry to the S2S endpoint with an app-only token for their own agent identity. The endpoint authorizes a token without OtelWrite when the agent is registered, which setup all already does for blueprint agents. Requesting OtelWrite (and its admin consent) is therefore unnecessary.

Validated live (dom97), sending through the real @microsoft/opentelemetry S2S exporter:

Agent identity Token Result
Registered, no role idtyp=app, roles=[], no scp 200, delivered to all sinks
Unregistered, no role same 403 insufficient_scope
Unregistered, OtelWrite role roles=[OtelWrite] 200

Behavior

  • Blueprint agents, every auth mode (obo, s2s, both): no Observability API in inheritable permissions, app-role grants, or admin consent URLs. The dry run and setup output say so. Registration is then the agent's only Observability authorization, so a registration failure is an error (exit 1).
  • --authmode s2s|both: still grant any other app-role specs (for example Defender, once Add Defender permissions part of "a365 setup all" #485 lands), but not OtelWrite; the S2S endpoint authorizes registered agents without it in every mode.
  • AI Teammate setup: unchanged. Instance creation in the admin center couldn't be validated end to end in the test tenant (it fails tenant-wide for unrelated agents too).
  • Fix: --agent-registration-only now exits 1 when registration fails.
  • Re-running setup does not revoke permissions granted earlier.

Release note

Ship this together with (or after) the SDK release containing microsoft/Agent365-nodejs#290. Agents on older SDKs that export through the delegated route need OtelWrite; grant it manually as described in the CHANGELOG upgrade note.

Testing

  • Full suite: 2026 passed, 12 skipped (pre-existing), 0 failed. The Release build has 0 warnings.
  • New and updated tests cover the default plan (omits Observability), s2s/both from the flag or config (keeps it), AI Teammate (keeps it), spec and consent-URL wiring, registration severity, and the summary. Each new test fails against a mutation of the line it guards.

Follow-ups

  • Decide the AI Teammate default once admin-center instance creation can be validated.
  • a365 setup permissions bot still configures Observability API.

Blueprint agents that export telemetry through the app-only S2S endpoint
(microsoft/Agent365-nodejs#290, microsoft/Agent365-Samples#339) are
authorized by their agent registration, so the Observability API OtelWrite
permission, and the admin consent it needs, is unnecessary for them.

- New opt-in `setup all --skip-observability-permissions` omits Observability
  API from the permission specs (inheritable permissions, app role grants,
  batch consent) and from the per-resource and combined admin consent URLs.
  Defaults are unchanged: the published SDKs still export to the non-S2S
  endpoint by default.
- The flag fails fast for AI Teammate agents and with authMode s2s/both,
  since OtelWrite is the only app role those modes grant. A contradicting
  --authmode flag is rejected before bootstrap signs in.
- With the flag, a failed agent registration is an error (exit 1), because
  registration is then the agent's only Observability authorization.
- Fix: `setup all --agent-registration-only` exited 0 when registration failed.
- Dry run plan, setup summary, CHANGELOG, and docs updated.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 5cbf5f6b-cc40-4b7e-a591-65848db73a12
Copilot AI lite review requested due to automatic review settings September 23, 2026 11:45
@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Sep 23, 2026
@github-actions

Copy link
Copy Markdown

⚠️ Deprecation Warning: The deny-licenses option is deprecated for possible removal in the next major release. For more information, see issue 997.

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Unresolved validation, failure-handling, persisted-consent, custom-permission, and route-documentation issues remain.

Review effort: Lite
Findings: None

What changed in this PR

Adds --skip-observability-permissions for blueprint setup and fixes registration-only failure handling.

Changes:

  • Adds permission, consent, validation, dry-run, and summary handling.
  • Updates registration failure severity and related tests.
  • Updates documentation and changelog.
File Description
src/​Tests/​Microsoft.Agents.A365.DevTools.Cli.Tests/​Helpers/​SetupHelpersDisplaySetupSummaryTests.cs Tests summary behavior.
src/​Tests/​Microsoft.Agents.A365.DevTools.Cli.Tests/​Commands/​SetupSubcommands/​PermissionSpecsTests.cs Tests permission specifications.
src/​Tests/​Microsoft.Agents.A365.DevTools.Cli.Tests/​Commands/​SetupCommandTests.cs Tests validation and dry-run behavior.
src/​Tests/​Microsoft.Agents.A365.DevTools.Cli.Tests/​Commands/​NonDwBlueprintSetupOrchestratorExecuteTests.cs Tests registration failure handling.
src/​Tests/​Microsoft.Agents.A365.DevTools.Cli.Tests/​Commands/​AllSubcommandTests.cs Tests permission and consent wiring.
src/​Microsoft.Agents.A365.DevTools.Cli/​Commands/​SetupSubcommands/​SetupResults.cs Tracks skipped permissions.
src/​Microsoft.Agents.A365.DevTools.Cli/​Commands/​SetupSubcommands/​SetupHelpers.cs Handles permissions and consent URLs.
src/​Microsoft.Agents.A365.DevTools.Cli/​Commands/​SetupSubcommands/​SetupContext.cs Stores setup options.
src/​Microsoft.Agents.A365.DevTools.Cli/​Commands/​SetupSubcommands/​README.md Documents the option.
src/​Microsoft.Agents.A365.DevTools.Cli/​Commands/​SetupSubcommands/​NonDwBlueprintSetupOrchestrator.cs Applies skip behavior and failure handling.
src/​Microsoft.Agents.A365.DevTools.Cli/​Commands/​SetupSubcommands/​AllSubcommand.cs Adds and validates the CLI option.
docs/​agent365-guided-setup/​a365-observability-instructions.md Updates observability guidance.
CHANGELOG.md Records the feature and fix.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes. The flag is well scoped and the incompatible-combination guards look right, but two gaps undercut the contract the docs promise ("setup exits with code 1 if registration fails"). Details inline. Both need a regression test.


// Registration is the sole purpose of --agent-registration-only and, with OtelWrite skipped,
// the agent's only Observability authorization, so its failure must fail setup.
var registrationRequired = skipIdentityAndPermissions || ctx.SkipObservabilityPermissions;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

registrationRequired covers the create-failure path, but the inconclusive-verification branch below (AgentRegistrationExistsAsync returns null, "retaining stored value") still sets registrationAlreadyExisted = true and setup exits 0. With --skip-observability-permissions registration is the agent's only authorization, so an auth or transient failure there should be an error rather than a pass. Please treat the null case as an error when registrationRequired is true, and add a test where the check returns null.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 9e72b1b. When registration is required (--agent-registration-only, or Observability permissions not requested, which is now every blueprint agent), a null from AgentRegistrationExistsAsync is recorded as an error and setup exits 1. The stored ID is kept and no duplicate registration is created. The optional path still retains the ID as before. Regression tests: Step6_RegistrationRequired_FailsWithoutReRegistering_WhenVerificationIsInconclusive covers both required modes, and the existing retain test now runs on the optional path with unchanged assertions.


var consentResourceNames = PopulateAdminConsentUrls(ctx.Config, mcpResourceAppId, mcpScopes, isM365, mcpScopesByAudience, mcpAudienceDisplayNames);
var includeObservability = !ctx.SkipObservabilityPermissions;
var consentResourceNames = PopulateAdminConsentUrls(ctx.Config, mcpResourceAppId, mcpScopes, isM365, mcpScopesByAudience, mcpAudienceDisplayNames, includeObservability);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PopulateAdminConsentUrls only upserts into ResourceConsents, so if an earlier run without the flag saved an Observability consent entry, it survives this run and is still surfaced to the admin. When includeObservability is false the Observability entry should be removed. A test that seeds the generated config with an existing Observability consent would cover it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 9e72b1b. When includeObservability is false, PopulateAdminConsentUrls now removes an Observability ResourceConsents entry saved by an earlier run. Regression test: PopulateAdminConsentUrls_WithoutObservability_RemovesObservabilityEntryFromEarlierRun seeds a stale entry and checks that the other resources are unaffected.

Per 3P Dev Scale scrum feedback, the no-consent flow becomes the main
path instead of an opt-in flag.

- Remove --skip-observability-permissions. Blueprint agents in the
  default (obo) auth mode no longer request Observability API
  permissions; registered agents export telemetry with an app-only
  token over the S2S endpoint.
- authMode s2s/both keep requesting OtelWrite, the only app role those
  modes grant; `both` also covers agents whose SDK still exports
  through the delegated (OBO) route.
- AI Teammate setup is unchanged until instance creation can be
  validated end to end.
- Registration failure stays an error on the default path.
- Tests: the default plan omits Observability; s2s/both (flag or
  config) keep it; AI Teammate keeps it. Mutation-checked.

Validated live: a roleless app-only token for a registered agent
identity exports 200 on S2S; an unregistered identity gets 403
insufficient_scope.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 5cbf5f6b-cc40-4b7e-a591-65848db73a12
Copilot AI review requested due to automatic review settings September 24, 2026 00:46
@DheerajPannala Krishnadheeraj (DheerajPannala) changed the title Add --skip-observability-permissions to setup all Stop requesting Observability API permissions for blueprint agents by default Sep 24, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity · 1 Low severity

Open (2)
Previously missed (1)

In code that hasn't changed since last review

Low severity Complete Observability API documentation sentence

src/​Microsoft.Agents.A365.DevTools.Cli/​Commands/​SetupSubcommands/​SetupHelpers.cs:1090

The generated API documentation is grammatically incomplete here: it renders as “Observability API unless ...” because the new text omits “is included.”

Comment thread src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs Outdated
Comment thread docs/agent365-guided-setup/a365-observability-instructions.md Outdated
The S2S endpoint authorizes registered agents without OtelWrite whatever
the auth mode, so s2s/both no longer request it either. They still grant
any other app-role specs (e.g. Defender once #485 lands). Agents whose SDK
still exports through the delegated route grant OtelWrite manually, as the
CHANGELOG upgrade note describes. AI Teammate setup is unchanged.

Tests encode the changed requirement for s2s/both (flag or config) and are
mutation-checked.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 5cbf5f6b-cc40-4b7e-a591-65848db73a12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Unresolved moderate findings affect documentation, AI Teammate dry-run behavior, and failure remediation.

Review effort: Lite
Findings: 1 Medium severity · 1 Low severity

Open (2)
Previously missed (1)

In code that hasn't changed since last review

Low severity Document conditional Observability exclusion in Step 4

src/​Microsoft.Agents.A365.DevTools.Cli/​Commands/​SetupSubcommands/​NonDwBlueprintSetupOrchestrator.cs:372

The Step 4 comment still says the permission-spec build stamps Observability, but this branch intentionally excludes it when SkipObservabilityPermissions is true. Keeping that description here makes the implementation contract misleading for future changes; state the conditional explicitly.

- When registration is required (--agent-registration-only, or Observability
  permissions not requested), an inconclusive registration check now fails
  setup instead of passing. The stored ID is kept and no duplicate
  registration is created. The optional path still retains the stored ID.
- When Observability is not included, drop an Observability consent entry
  saved by an earlier run so the admin is not asked for it.
- Keep Observability for an AI Teammate config retained for a dry run (skip
  only for an effective blueprint selection).
- Scope the guided-setup OtelWrite grant steps to AI Teammates and SDKs that
  still export through the delegated route; fix two stale doc comments.

Regression tests cover each case and are mutation-checked.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 5cbf5f6b-cc40-4b7e-a591-65848db73a12
@DheerajPannala

Copy link
Copy Markdown
Contributor Author

Review feedback addressed in 9e72b1b:

  • Registration (Rick): an inconclusive registration check now fails setup when registration is required, keeping the stored ID and not re-registering.
  • Consent (Rick): a stale Observability consent entry from an earlier run is removed when Observability isn't requested.
  • AI Teammate dry run (Copilot): Observability is still requested for AI Teammate configs, including those kept for a dry run.
  • Docs (Copilot): the guided-setup grant steps are scoped to AI Teammates and older delegated-route SDKs. The two stale doc comments are fixed.

Each fix has a regression test that fails against a mutation of the fix. Full suite: 2030 passed, 12 skipped (pre-existing), 0 failed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity · 1 Low severity

Open (2)
Resolved since last review (2)

Comment thread CHANGELOG.md Outdated
Make the upgrade note one consumer-facing sentence, and update the Fixed
entry: setup exits 1 when registration fails or cannot be verified for
blueprint agents as well as with --agent-registration-only. Replace
"without the flag" in a registration test, since the flag was removed.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 5cbf5f6b-cc40-4b7e-a591-65848db73a12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

A moderate stale-consent cleanup issue remains, along with two documentation/comment nits.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Low severity

Open (1)
Resolved since last review (2)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Remove stale Observability consent URLs before early return

src/​Microsoft.Agents.A365.DevTools.Cli/​Commands/​SetupSubcommands/​SetupHelpers.cs:1451

When a prior non-admin run persisted an Observability consent URL, a subsequent run with Observability skipped can leave that stale entry behind whenever TenantWideConsentOutcome is Granted (or the blueprint ID is absent), because this method returns before PopulateAdminConsentUrls performs the removal. The generated config can therefore still advertise an admin-consent URL for a permission this run did not request; move the stale-entry cleanup before this early return (while preserving any intentional record-retention policy).

Comment thread CHANGELOG.md Outdated
The upgrade note opened by saying every existing agent needs the
Observability permissions, which contradicted the S2S exception. Scope
the heading and requirement to agents that export through the delegated
(OBO) route.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 5cbf5f6b-cc40-4b7e-a591-65848db73a12
Copilot AI review requested due to automatic review settings September 24, 2026 13:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

One or more issues must be addressed before approval.

Review effort: Lite
Findings: None

Resolved since last review (1)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants